1488417 - Even better error message on property access on undefined/null variable

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1259822, we improved the error message when accessing a property on an undefined/null variable which is great.
Some people suggested that the new message might be better phrased : 
- https://twitter.com/stevedesmond_ca/status/1035570502784434176

It would be nice if we could do it soon and uplift it to beta.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Jason, since you review the patch for Bug 1259822 and are a native english speaker (I think), do you have a sentence that could feel better ? Thanks !

Comment 2

[Florens Verschelde :fvsch]

The simplest change could be to remove the "of it", inferring the relationship from context:

> TypeError: x is undefined; cannot access property "y"

Other options:

> TypeError: x is undefined; cannot access its property "y"
> TypeError: x is undefined; cannot access "x.y"

(I also think it might be more correct to use a semi-colon instead of a coma, to avoid a possible reading as "x cannot access y", and to use "cannot" because errors tend to sound a bit formal.)

Comment 3

[Jason Orendorff [:jorendorff]]

I couldn't think of anything at the time of code review, so I let it go. "cannot access its property 'y'" seems good, so I'll do that...

Comment 4

[Jason Orendorff [:jorendorff]]

I decided `can't access its "y" property` sounds a little better, especially since the message is more likely to be `its "firstChild" property` or some other identifier made of actual words. We already use the phrase `its [Symbol.toPrimitive] property` in some other error messages.

But this makes the message worse for code like

    undefined[2]

so I'm splitting this into two error messages, one for string/symbol keys and one for integer keys:

    TypeError: a is undefined; can't access its "length" property
    TypeError: a is undefined; can't access element 2

I'm not totally pleased with the latter, but "its 2 property" is worse.  ("Element" is a standard term for indexed properties; people talk about "array elements" and so forth.)

Comment 5

[Jason Orendorff [:jorendorff]]

Running tests. We have surprisingly many that care about the exact wording of this error message.

----

Regarding the word "cannot", it's a bug everywhere that appears in js.msg. It's true that many software systems use that peculiarly formal register in their error messages, but SpiderMonkey has always avoided it. In short, people don't talk like that. Consider the four other cases where you might say "cannot":

1. you're a police officer speaking at a press conference;

2. you're a technical standard, and for some technical reason (even more technical than usual) "MUST NOT" would be wrong in this particular case;

3. you're an android on a TV show, and although you can do triple integrals in your head while playing the violin, you somehow never got the hang of contractions;

4. you're not exactly human, your part was written to sound old and rich with power by a linguist who's fluent in Middle English, and you're speaking to a Balrog.

If the event of an actual Balrog-level emergency, I could be persuaded to go with "cannot"; but for ordinary JS TypeErrors, none of these is quite the vibe we're looking for.

Comment 6

[Jason Orendorff [:jorendorff]]

> Regarding the word "cannot", it's a bug

Filed bug 1488588.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Even better error message on property access on undefined/null variable

Comment 8

[Anthony Ricaud (:rik)]

Another suggestion for string/symbol keys:
TypeError: Can't access property "length" of a; a is undefined

Another suggestion for integer keys:
TypeError: a is undefined; can't access element at index 2

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1488417 - Even better error message on property access on undefined/null variable. r=Waldo

Comment 10

[Jason Orendorff [:jorendorff]]

Ooh, I like "at index 2"; I've thrown that in and updated the patch.

We discussed this message ad nauseum in #jsapi yesterday. Some points made were:

* Leading with `a is undefined` or `a is null` seems best. That is the most important thing for the user to understand, whether they're new to JS or experienced.

* The expression that's null or undefined probably shouldn't appear multiple times in the error message. It can look pretty weird when it's more than a single character.

* It is weirdly easy to write a message that is ambiguous (can't tell if it's saying that `a` is undefined or that `a.length` is undefined), though perhaps only confusing for JS newbies.

I'm happy with where we ended up here, semicolon and all.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 9007040 [details]
Bug 1488417 - Even better error message on property access on undefined/null variable. r=Waldo

Jeff Walden [:Waldo] has approved the revision.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cfd695493ec7
Even better error message on property access on undefined/null variable. r=jwalden

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out for causing mochitest failures on test_browserElement_inproc_ExecuteScript.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=Windows%207%20debug%20Mochitests%20test-windows7-32%2Fdebug-mochitest-chrome-2%20M(c2)&fromchange=cfd695493ec7abcf8a3a0d7c37903c7bbe598b5c&tochange=2bdf336471fdb46b34049aca0edb89906a3bd6d1&selectedJob=198649276

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198649276&repo=autoland&lineNumber=1546
TV failures - https://treeherder.mozilla.org/logviewer.html#?job_id=198646636&repo=autoland&lineNumber=12658

Backout link: https://hg.mozilla.org/integration/autoland/rev/2bdf336471fdb46b34049aca0edb89906a3bd6d1

14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 8 
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 9 
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 10 
14:04:07     INFO - Buffered messages finished
14:04:07     INFO - TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 11 - got "TypeError: window.wrappedJSObject is undefined; can't access its \"btoa\" property", expected "TypeError: window.wrappedJSObject is undefined, can't access its \"btoa\" property"
14:04:07     INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:312:5
14:04:07     INFO - onReady/<@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_ExecuteScript.js:94:7
14:04:07     INFO - promise callback*onReady@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_ExecuteScript.js:93:14
14:04:07     INFO - runTest/</<@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_ExecuteScript.js:32:7
14:04:07     INFO - _fireEventFromMsg@jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js:348:5
14:04:07     INFO - _fireProfiledEventFromMsg@jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js:328:5
14:04:07     INFO - receiveMessage@jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js:197:14
14:04:07     INFO - MessageListener.receiveMessage*_setupMessageListener@jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js:140:5
14:04:07     INFO - setFrameLoader@jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js:115:5
14:04:07     INFO - runTest/<@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_ExecuteScript.js:35:5
14:04:07     INFO - setTimeout handler*_setTimeout@resource://specialpowers/specialpowersAPI.js:762:7
14:04:07     INFO - pushPermissions@resource://specialpowers/specialpowersAPI.js:891:7
14:04:07     INFO - runTest@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElement_ExecuteScript.js:25:3
14:04:07     INFO - unlockTestReady@chrome://mochitests/content/chrome/dom/browser-element/mochitest/browserElementTestHelpers.js:47:7
14:04:07     INFO - GECKO(1520) | JavaScript error: jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js, line 622: NS_ERROR_ILLEGAL_VALUE: Invalid argument
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 12 
14:04:07     INFO - GECKO(1520) | JavaScript error: jar:file:///Z:/task_1536673592/build/application/firefox/omni.ja!/components/BrowserElementParent.js, line 622: NS_ERROR_ILLEGAL_VALUE: Invalid argument
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 13 
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 14 
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 15 
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 16 
14:04:07     INFO - TEST-PASS | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | scriptId: 17 
14:04:07     INFO - GECKO(1520) | MEMORY STAT | vsize 726MB | vsizeMaxContiguous 530MB | residentFast 244MB | heapAllocated 100MB
14:04:07     INFO - TEST-OK | dom/browser-element/mochitest/test_browserElement_inproc_ExecuteScript.html | took 406ms

[task 2018-09-11T13:44:27.043Z] 13:44:27     INFO -  TEST-START | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_create_iframe.js
[task 2018-09-11T13:44:31.381Z] 13:44:31  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_create_iframe.js | xpcshell return code: 0
[task 2018-09-11T13:44:31.382Z] 13:44:31     INFO -  TEST-INFO took 4337ms
[task 2018-09-11T13:44:31.383Z] 13:44:31     INFO -  >>>>>>>
[task 2018-09-11T13:44:31.384Z] 13:44:31     INFO -  PID 7021 | [7021, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2727
[task 2018-09-11T13:44:31.385Z] 13:44:31     INFO -  PID 7021 | JavaScript strict warning: resource://testing-common/AddonTestUtils.jsm, line 304: ReferenceError: reference to undefined property "testScope"
[task 2018-09-11T13:44:31.386Z] 13:44:31     INFO -  PID 7021 | JavaScript strict warning: resource://testing-common/AddonTestUtils.jsm, line 310: ReferenceError: reference to undefined property "testScope"
[task 2018-09-11T13:44:31.387Z] 13:44:31     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "testScope"" {file: "resource://testing-common/AddonTestUtils.jsm" line: 304}]"
[task 2018-09-11T13:44:31.388Z] 13:44:31     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "testScope"" {file: "resource://testing-common/AddonTestUtils.jsm" line: 310}]"
[task 2018-09-11T13:44:31.389Z] 13:44:31     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-09-11T13:44:31.390Z] 13:44:31     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-09-11T13:44:31.391Z] 13:44:31     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-09-11T13:44:31.392Z] 13:44:31     INFO -  running event loop
[task 2018-09-11T13:44:31.392Z] 13:44:31     INFO -  xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contentscript_create_iframe.js | Starting check_remote
[task 2018-09-11T13:44:31.393Z] 13:44:31     INFO -  (xpcshell/head.js) | test check_remote pending (2)

Comment 14

[Cosmin Sabou [:CosminS]]

Those TV failures have now turned up also on tier here: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=96386cdfaf65bdb9d33435509ead8932e4ef6e2f&filter-searchStr=Android%204.3%20API16%2B%20debug%20Xpcshell%20tests%20test-android-em-4.3-arm7-api-16%2Fdebug-xpcshell-11%20X(X11)&selectedJob=198648263

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198648263&repo=autoland&lineNumber=1385

Comment 15

[Jason Orendorff [:jorendorff]]

::sigh::

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1488417 - Even better error message on property access on undefined/null variable. r=Waldo

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b58b7cd29a0b
Even better error message on property access on undefined/null variable.

Comment 18

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/b58b7cd29a0b

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Could this patch be uplifted to beta ?
We're seeing some issue in scripts/libraries where author rely on a given Error message (See Bug 1490772, https://github.com/facebookincubator/idx/issues/53, …).
It would be nice if we won't make change their code for 63 AND 64 (would be nicer if they don't do that but that's not in our power I guess).

Jason what do you think ?

Comment 20

[Jason Orendorff [:jorendorff]]

This won't be uplifted. The bar is much higher than this. Many actual bug fixes don't qualify.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out from mozilla-release (= Gecko 63) and mozilla-beta (= Gecko 64 in beta repo) as requested by Jason:

https://hg.mozilla.org/releases/mozilla-beta/rev/7a95803c70dd4294d9127f90f11d6c4e438cbacf
https://hg.mozilla.org/releases/mozilla-beta/rev/db3a5881e0d22fa59c2107d49c0fefa675fe6bd3 (FIREFOX_63b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/ea4b5c999c575532506dfe6c702f59c5a268983b

Setting needinfo to set status for firefox65 to fixed once it gets added next week.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Only this backout is relevant for this bug:
https://hg.mozilla.org/releases/mozilla-beta/rev/7a95803c70dd4294d9127f90f11d6c4e438cbacf

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out the backout (=relanded) due to bug913749.js failing:

Bug 1488417: https://hg.mozilla.org/releases/mozilla-beta/rev/0413c9c3d902bf4653569bd4c0e7e6ba1e239ba8
Bug 1259822: https://hg.mozilla.org/releases/mozilla-beta/rev/35c61888a49d69506cdd330b81885838ccf45f8c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205877473&repo=mozilla-beta
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=usercancel%2Crunnable%2Ctestfailed%2Cbusted%2Cexception%2Cretry&group_state=expanded&selectedJob=205877473&revision=f76d65097eb36e929926ebf9ebe86052bbd2f67e

Means it fails this change:
https://hg.mozilla.org/releases/mozilla-beta/rev/0e99081b5322d213fdba77a12ebbf6293f9c2a7f#l8.1

Comment 24

[Julien Cristau [:jcristau] (back April 22)]

Backed out once more:
https://hg.mozilla.org/releases/mozilla-release/rev/290d09651022f524b7bcc5a5514dc616f93fe89a

Comment 25

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a738863362f2584843db9af578c2c1ff4b9b03a0

Comment 26

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc0458ea0172a766793998bf38fad8f2e203212
Backed out 2 changesets (bug 1488417, bug 1259822) for web compat issues.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This was backed out from all releases for various web compat regressions. Calling this WONTFIX per IRC discussion with jorendorff.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Given that our message still doesn't match Chrome's (and in fact per https://news.ycombinator.com/item?id=19493433 contains completely disjoint information!), could we consider changing it again to something that would pass the regexps we have seen but still includes the information we want to include? The high-level bit seemed to be that the string needs to end with "undefined", right?

We'd probably want a few cycles of nightly testing first, but might still be worth it...

Comment 29

[Jason Orendorff [:jorendorff]]

The safest change is to converge on Chrome's error message.

The next-safest course is, of course, to change nothing.

I don't want to be overly timid here, but anything else risks being a big waste of everyone's time, and finally being backed out, thus achieving nothing. Feel free; as for me, I have other things to work on...