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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
7 months ago
3 months ago

People

(Reporter: nchevobbe, Assigned: jorendorff)

Tracking

(Depends on: 1 bug)

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 months ago
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.
(Reporter)

Comment 1

7 months ago
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 !
Flags: needinfo?(jorendorff)
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.)
(Assignee)

Comment 3

7 months ago
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...
Flags: needinfo?(jorendorff)
(Assignee)

Comment 4

7 months ago
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.)
(Assignee)

Comment 5

7 months ago
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.
(Assignee)

Comment 6

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

Filed bug 1488588.
(Assignee)

Updated

7 months ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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
(Assignee)

Updated

7 months ago
Attachment #9006404 - Attachment is obsolete: true
Attachment #9006404 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 10

7 months ago
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 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.
Attachment #9007040 - Flags: review+

Comment 12

6 months ago
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfd695493ec7
Even better error message on property access on undefined/null variable. r=jwalden
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)
Flags: needinfo?(jorendorff)
(Assignee)

Comment 15

6 months ago
::sigh::

Comment 17

6 months ago
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b58b7cd29a0b
Even better error message on property access on undefined/null variable.

Comment 18

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b58b7cd29a0b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
status-firefox63: affected → wontfix
Flags: needinfo?(jorendorff)
(Reporter)

Comment 19

6 months ago
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 ?
(Assignee)

Comment 20

6 months ago
This won't be uplifted. The bar is much higher than this. Many actual bug fixes don't qualify.
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.
status-firefox64: fixed → wontfix
Flags: needinfo?(aryx.bugmail)
(Assignee)

Updated

5 months ago
Depends on: 1498257
status-firefox64: fixed → wontfix
status-firefox65: --- → fixed
Flags: needinfo?(aryx.bugmail)
Target Milestone: mozilla64 → mozilla65
Depends on: 1510193
This was backed out from all releases for various web compat regressions. Calling this WONTFIX per IRC discussion with jorendorff.
status-firefox65: fixed → wontfix
status-firefox66: --- → wontfix
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.