Closed Bug 1488417 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix

People

(Reporter: nchevobbe, Assigned: jorendorff)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1488417 - Even better error message on property access on undefined/null variable. r=Waldo
46 bytes, text/x-phabricator-request
Waldo
: review+
Details | Review
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.
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.)
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)
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.)
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.
> Regarding the word "cannot", it's a bug Filed bug 1488588.
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
Attachment #9006404 - Attachment is obsolete: true
Attachment #9006404 - Flags: review?(jwalden+bmo)
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+
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)
::sigh::
Pushed by jorendorff@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b58b7cd29a0b Even better error message on property access on undefined/null variable.
https://hg.mozilla.org/mozilla-central/rev/b58b7cd29a0b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
status-firefox63: affected → wontfix
Flags: needinfo?(jorendorff)
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 ?
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)
Depends on: 1498257
status-firefox64: fixed → wontfix
status-firefox65: --- → fixed
Flags: needinfo?(aryx.bugmail)
Target Milestone: mozilla64 → mozilla65
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

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...

Flags: needinfo?(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...

Flags: needinfo?(jorendorff)
Attachment #9008560 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: